-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix the bug with scrolling in Virtualize component with scaled elements #64013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the Virtualize component where scroll calculations were incorrect when CSS scaling transforms were applied to parent elements. The issue occurred because the component used physical pixel measurements without accounting for scale factors.
- Adds scale factor detection and compensation logic to handle CSS scale, zoom, and transform properties
- Converts physical pixel measurements to logical pixels for accurate virtualization calculations
- Includes test coverage for the scaling scenario with a new E2E test
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
VirtualizationScale.razor | New test component demonstrating virtualization with CSS scale applied to body |
QuickGridVirtualizeComponent.razor | Fixes items provider to respect pagination parameters |
Index.razor | Adds new test component to navigation options |
VirtualizationTest.cs | Adds E2E test to verify virtualization works with scaling |
Virtualize.ts | Core fix - adds scale factor detection and applies compensation to scroll calculations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! The comments are mostly optional changes/questions.
rangeBetweenSpacers.setStartAfter(spacerBefore); | ||
rangeBetweenSpacers.setEndBefore(spacerAfter); | ||
const spacerSeparation = rangeBetweenSpacers.getBoundingClientRect().height; | ||
const rangeRect = rangeBetweenSpacers.getBoundingClientRect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is redundant, rangeRect
is not used anywhere.
|
||
<style> | ||
body { | ||
scale: 2 0.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in the algo we focus on y-axis value, the tests check mainly 50% scaling (decreasing). Would such an approach work, so that we can test increasing and decreasing cases?
@code {
[Parameter]
public double ScaleX { get; set; } = 1.0;
[Parameter]
public double ScaleY { get; set; } = 1.0;
}
<style>
body {
scale: @ScaleX, @ScaleY;
transform-origin: top left;
}
</style>
I'm not sure how to pass these params to the component, though. The component is mounted directly, not navigated to. Maybe we can add path
to it and then use SetUrlViaPushState("/path?scaleX=1&scaleY=1");
to pass the query values?
Skip if it's too much test logic changes and you tested both cases. We can always add additional tests after the fix lands.
} | ||
|
||
[Fact] | ||
public void CanScrollWhenAppliedScale() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this fail without the change? The check that I used for detecting the glitch was much more complex. Some solutions did not prevent the full loading of data but still, resulted in the glitch on the way to the bottom.
// Check for transform property (matrix form) | ||
if (computedStyle.transform && computedStyle.transform !== 'none') { | ||
// A 2D transform matrix has 6 values: matrix(scaleX, skewY, skewX, scaleY, translateX, translateY) | ||
const match = computedStyle.transform.match(/matrix\([^,]+,\s*[^,]+,\s*[^,]+,\s*([^,]+)/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the bug with scrolling in Virtualize component with scaled elements
Description
This pull request improves the
Virtualize
component’s handling of CSS scaling and transforms, ensuring correct virtualization behavior when parent elements use scale, zoom, or transform properties. It also adds new test to verify these changes.The issue was caused by inconsistent scaling algorithm between JS and C#. JS used to send scaled size values to C# while C# logic assumes they are not scaled. This PR standardizes the sizes before sending to reflect non-scaled values.
Changes:
Virtualize.ts
to detect and correctly handle CSS scale, zoom, and transform properties on parent elements, adjusting scroll calculations so virtualization works accurately under scaling scenarios.CanScrollWhenAppliedScale
to verify that virtualization works when CSS scale is applied.Fixes #59354